-
Notifications
You must be signed in to change notification settings - Fork 28
Conversation
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
=======================================
Coverage 51.56% 51.56%
=======================================
Files 63 63
Lines 3105 3105
=======================================
Hits 1601 1601
Misses 1504 1504
Continue to review full report at Codecov.
|
configs/docker-release.toml
Outdated
@@ -23,3 +23,7 @@ model_type = "M3" | |||
|
|||
[model] | |||
size = 4 | |||
|
|||
[metrics] | |||
store_url = "http://influxdb:8086" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that some settings will be specific to the metrics backend we're using. Maybe we have different settings for each backend:
[metrics.influxdb]
url = "http://influxdb:8086"
store_name = "metrics"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point, I think namespacing [metrics.influxdb]
is indeed useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good👍
Do you know if we can make these settings optional? I mean for the whole settings object we could use #[cfg(feature = "metrics")]
but I think it is nicer if we can just make it optional.
If you like you can also add the fields username
and password
. Both have the type String
. But here it is important that both fields are optional because influx allows communication without authentication.
rust/src/settings.rs
Outdated
/// ```text | ||
/// XAYNET_API__STORE_URL=http://localhost:8086 | ||
/// ``` | ||
pub store_url: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if it's easy to check if the value is a valid URL?
For example you can use the type std::net::SocketAddr
for ip addresses.
When the configuration file is parsed, SocketAddr::FromStr
is called and it will automatically fail if it is not a valid IP address. We used that behaviour for ApiSettings::bind_address
. Maybe there is something similar for urls in the std lib.
However, if it requires an additional library, I'm not sure it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can easily check whether a URL is valid, whether is contains a hostname, what the scheme is, etc. But we cannot check that the domain is valid in that it resolves to an IP address without making a network call, and I think we should refrain from doing network calls when parsing a config file, because they can hang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing we can certainly do I think is #[validate(url)]
. I'll add that now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I just thought of checking that scheme is valid.
But maybe it's not that important for now (just an idea I had in mind).
thanks for reviewing. would you be happy if we postpone the "optionality" of the settings to another PR? because I had in mind other settings (in PET, especially) which should also be optional, so it could all be tackled there. |
Sounds good to me 👍 |
Edit: sorry as I pressed "comment" I realized I had mis-read your comment. You're actually proposing not to hide the settings behind that flag. |
Adds a settings struct for
[metrics.influxdb]
settings